Timeseries stubs - #17
Conversation
agavra
left a comment
agavra
left a comment
There was a problem hiding this comment.
The actual proposal LGTM but this PR creates a large fork from the code that's already in opendata/timeseries. I've commented in-line about where the equivalent items exist in the current implementation.
| /// - [`Internal`](Error::Internal): Unexpected internal errors that indicate bugs | ||
| /// or invariant violations. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum Error { |
There was a problem hiding this comment.
we should consider naming it TimeseriesError so every db doesn't have an error class named Error. We also already have OpenTsdbError in util.rs, we should consolidate the two
| //! ```ignore | ||
| //! use timeseries::{TimeSeries, Config, Series}; | ||
| //! | ||
| //! #[tokio::main] |
There was a problem hiding this comment.
should we update main.rs to match or just remove it altogether?
| } | ||
|
|
||
| /// Adds a label to the series. | ||
| pub fn label(mut self, name: impl Into<String>, value: impl Into<String>) -> Self { |
There was a problem hiding this comment.
consider add_label and add_sample to make it clear that you can add multiple (as opposed to something like unit which overwrites)
| @@ -0,0 +1,143 @@ | |||
| //! Core TimeSeries implementation with write API. | |||
There was a problem hiding this comment.
should this just replace tsdb.rs or are you planning on using that internally here?
| @@ -0,0 +1,319 @@ | |||
| //! Core data types for OpenData TimeSeries. | |||
There was a problem hiding this comment.
we should consolidate this and what's already in model.rs -- also note that in your comment "Migrate OTEL implementation on top of new API" the current implementation in this repo is already very close to this, it doesn't use OTEl at all (see tsdb.rs which has a main ingest API of pub(crate) async fn ingest_samples(&self, samples: Vec<SampleWithAttributes>))
I think it'd probably be easier to rename the existing model.rs structs (e.g. SampleWithAttributes to Series, and Attribute to Label) to match your proposal so the rest of the code just comes along for the ride.
| @@ -0,0 +1,76 @@ | |||
| //! Configuration options for OpenData TimeSeries operations. | |||
There was a problem hiding this comment.
Note that we already have promql/config.rs, which is the Server configuration
@agavra Yep, I understood there was some duplication. The rough plan I had is in the description. We could cut straight through some of it if you prefer. Usually I find it a bit easier to start with the API that you want and consolidate internally rather than trying to factor out the API from the internal implementation, but I don't think it makes a dramatic difference. |
got it, that makes sense to me. I wanted to make sure you had seen those other components and to point out which ones refer to what so it's easier when we do consolidate them. I have no preference on the order of which we do it so we can merge this PR with the duplication for now. |
|
Makes sense. Let me take a quick look and see if there are any obvious candidates that can be consolidated now with minimal refactoring. |
|
@agavra The label/attribute consolidation was straightforward (just renaming), so I pushed that into this patch. I suggest we do the rest in subsequent patches. How does that sound? |
2 participants
Adds stubbing for timeseries RFC 2. I decided to flatten SeriesMetadata to simplify the API. I also made some other minor changes to the RFC. I imagine the following evolution of the implementation internally: